[AURON #1911][clippy] Enable -D warnings and resolve related issues #1922
Conversation
ShreyeshArangath
left a comment
ShreyeshArangath
left a comment
There was a problem hiding this comment.
LGTM, thanks for fixing this
There was a problem hiding this comment.
Pull request overview
This PR enables stricter Clippy linting in the CI pipeline by switching from allowing all warnings to denying all warnings by default, while selectively allowing specific lints that require more extensive refactoring to fix. The PR also resolves many auto-fixable clippy warnings throughout the codebase and annotates existing panic-related code with #[allow(clippy::panic)] attributes.
Changes:
- Configure workspace-level lints with
unwrap_usedandpanicdenied, while temporarily allowing other lints that need gradual refactoring - Update CI and build scripts to use
cargo clippy --all-targets --workspace -- -D warningsinstead of selectively denying onlyunwrap_used - Apply auto-fixes for numerous clippy lints including
needless_borrow,redundant_closure,needless_return,useless_format,uninlined_format_args,manual_range_contains,into_iter_on_ref, and others - Add
#[allow(clippy::panic)]annotations to existing panic-heavy code with comments indicating future refactoring plans
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Add workspace-level lint configuration denying unwrap_used and panic, while temporarily allowing other lints |
| dev/mvn-build-helper/build-native.sh | Update clippy command to deny all warnings instead of only unwrap_used |
| .github/workflows/tpcds-reusable.yml | Update clippy command in CI workflow to deny all warnings |
| native-engine/*/Cargo.toml | Add [lints] workspace = true to all sub-crates |
| native-engine/datafusion-ext-plans/src/**/*.rs | Add panic allow annotations to existing panic-heavy code |
| native-engine/datafusion-ext-functions/src/**/*.rs | Apply auto-fixes for clippy lints and add panic annotations |
| native-engine/datafusion-ext-exprs/src/**/*.rs | Apply auto-fixes for needless borrows, redundant closures, and other clippy suggestions |
| native-engine/datafusion-ext-commons/src/**/*.rs | Apply extensive auto-fixes including format strings, redundant references, and test improvements |
| native-engine/auron*/src/**/*.rs | Apply auto-fixes for format strings, needless returns, and const thread_local initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int_plus_one = "allow" | ||
| derived_hash_with_manual_eq = "allow" | ||
| approx_constant = "allow" | ||
| op_ref = "allow" |
There was a problem hiding this comment.
The PR description states that auto-fixable lint issues are being resolved via cargo clippy --fix, and many clippy warnings have indeed been fixed in the code (e.g., needless_return, redundant_closure, useless_format, uninlined_format_args). However, these same lints are configured as "allow" in the workspace lint configuration with a comment "# Pending processing (temporarily allow)".
This creates a confusing situation where the code has been fixed for these lints, but they're still allowed. Consider either:
- Removing the "allow" directives for lints that have already been fixed throughout the codebase, or
- Clarifying in the PR description and commit message that this is a two-phase approach where fixes are applied first, but enforcement will come in a future PR.
If the intent is to gradually enforce these lints, it would be beneficial to track which lints still need fixes versus which are already clean.
| op_ref = "allow" | |
| op_ref = "warn" |
3 participants
Which issue does this PR close?
Closes #1911
Rationale for this change
What changes are included in this PR?
-D warningsfor Clippy in CI, and setworkspace.lints = truefor all sub-crates to align with unified workspace lint config.cargo clippy --fix.#[allow(clippy::panic)]annotationAre there any user-facing changes?
How was this patch tested?